Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Record embed and const field syntax #474

Closed
wants to merge 4 commits into from

Conversation

pigpigyyy
Copy link
Contributor

@pigpigyyy pigpigyyy commented Sep 6, 2021

Implement the "embed" and "const / readonly" field functions for OOP simulations to support some Lua project uses. The "embed" function is refactored from @euclidianAce's implementation ( mentioned in issue #97 ) with bug fixes.

@pigpigyyy pigpigyyy changed the title Readonly field Record embed and const field syntax Sep 6, 2021
@github-actions
Copy link

github-actions bot commented Sep 6, 2021

Teal Playground URL: https://474--teal-playground.netlify.app

@hishamhm
Copy link
Member

hishamhm commented Sep 9, 2021

Hi @pigpigyyy, thank you for taking the time to make a PR!

The code itself looks good, but I'm uncertain about adding these features to the language. These are the reasons:

  • The embed feature seems to suggest a structural subtyping system, but Teal records are nominal. I'm thinking of adding explicit subtyping, but that needs cleaning up the logic for covariant/contravariant checks — since Lua tables are used in both modes, there would need to be some annotation for covariant/contravariant arguments, and making this in a non-confusing manner for using is an interesting design problem.

  • The readonly feature for record fields is interesting, but I'm afraid it might provide a false sense of security, since fields could be rewritten by simply aliasing the record to a different type...

I'd like to hear what you and other users think about these concerns!

@lenscas
Copy link
Contributor

lenscas commented Sep 26, 2021

The readonly feature for record fields is interesting, but I'm afraid it might provide a false sense of security, since fields could be rewritten by simply aliasing the record to a different type...

In most languages there are also ways to get around fields being public, private and/or protected. Yet those languages don't give that idea up and properly setting the access for fields is encouraged instead of having everything be public.

Also, by that same logic, the entire type system gives a false sense of security as a simple as can be used to give a table to a function that takes an integer.

I think it is best to treat as as how Rust treats Transmute, it is there if you really need/want it but... YOU need to make sure you don't screw up.

@pigpigyyy
Copy link
Contributor Author

pigpigyyy commented Sep 27, 2021

I'm using Teal in a C++ project with Lua embedded, and get a lot of native OOP stuff exported into Lua. So I just seek for a way to do the simulated OOP check. Since C++ does not support covariant/contravariant features by default, I'm not worrying too much for it. As for nominal records, I just changed the way to compare records with embeds in order to make subtypes to be not equal.

@hishamhm
Copy link
Member

I think it is best to treat as as how Rust treats Transmute, it is there if you really need/want it but... YOU need to make sure you don't screw up.

@lenscas What I meant by aliasing is different from an explicit as because there are places where we have bivariant behavior (due to lack of proper support for record subtyping yet) so that causes implicit aliasing. I agree that in explicit as, all bets are off. My concern was about implicit aliasing and changing those rules in the future, hope that clarifies!

@hishamhm
Copy link
Member

As for nominal records, I just changed the way to compare records with embeds in order to make subtypes to be not equal.

Thanks @pigpigyyy, I'll give it another review!

@lenscas
Copy link
Contributor

lenscas commented Sep 27, 2021

I think it is best to treat as as how Rust treats Transmute, it is there if you really need/want it but... YOU need to make sure you don't screw up.

@lenscas What I meant by aliasing is different from an explicit as because there are places where we have bivariant behavior (due to lack of proper support for record subtyping yet) so that causes implicit aliasing. I agree that in explicit as, all bets are off. My concern was about implicit aliasing and changing those rules in the future, hope that clarifies!

Ah, yes then I fully agree to wait until that to see if readonly is a good fit.

@muratg
Copy link

muratg commented May 27, 2022

@hishamhm Did you get a chance to look into this a bit more? I'm personally loving the "embed" design and implementation here. It's feels almost like intersection types from TypeScript.

I'm not worried about type-soundness, but being able to express another much-common usage of tables in the type system.
I'm considering taking this PR in a temporary fork if you're likely get this or a something similar PR in the future. (I'm currently using TypescriptToLua but taking a serious look at Teal.)

// ETA: Some idle discussion about syntax aside. Not directly related to the PR. BIG changes proposed as thought experiments //

  • Consider swapping syntax {string:number} with {[string]number} (":" doesn't make sense in the former anyway.)
  • Tuples and arrays stay the same {integer}; {number, boolean}
  • Consider anonymous record type: {a: integer, b: number} (more intuitive IMHO)
  • Consider swapping syntax function(integer,number):number with function(integer,number)->number, (":" doesn't make sense..)
    • (Or even just (integer,number)->number...)
  • Implement intersection types (or something like it)
    • "extend"/"embed" syntax can just be "&" types
    • Array records: {integer} & {a:integer, b:number}
    • Userdata: Userdata & {x: number, t: boolean} (Or something similar... )
  • Enums can be "|" types. local type SwitchStatus = "on" | "off"
  • Perhaps keep the current syntax "deprecated" for a while
  • A parser/prettyprinter combo can keep help write "converter" tools to update "deprecated" syntax

Not sure if syntax discussions are closed for good or not, but these are some thoughts coming from TypeScript (specifically using it to compile Lua) pov. HTH.

// ETA 2 (Can you tell I'm passionate about this? 😄) Some more realistic (but probably not enough) syntax ideas around types. //

local type Pos = record {x:number, y:number}
local type Size = record {w:number, h:number}
local type Color  = record {r:number, g:number, b:number, a:number}
-- example with "embedded" types, similar to intersection types of TS
-- this maybe easier to implement then `Pos & Size & ....` style syntax, which may require a new `typealias` directive.
local type MyItem = record(Pos, Size, Color, {max_x: number, maX_y: number})
local type MyOption = enum("on","off")

// ETA 3 -- Updated opinions after exploring the ecosystem //

While I still like the syntax I suggested in previous edits, I think it would be unrealistic since there seems to be a big enough userbase to break. Not sure if they're implementable without breaking existing code so I'm changing my thinking on this. I think the syntax implemented in this PR seems fine.

I now recommend that

  • readonly/const parts are removed
  • rest is cleaned a bit (there's a few small issues)

If @hishamhm accepts this, I'd be happy to the a full review of this PR, and or update it if @pigpigyyy is not interested in this anymore.

Thanks.

@hishamhm
Copy link
Member

@muratg Thanks for the feedback!

Regarding this feature proposal, I think the direction Teal should go is to adopt real intersection types. Various features in the language already hint in this direction (arrayrecords, polymorphic function members in records) so it would be cleaner to replace them with intersection types. I'm sure that intersections would end up having some limitations similar to those we currently have with unions, but I think overall it would make the design cleaner (and the language hopefully smaller, which is not a hard goal but always a hint of elegance).

Regarding the other syntax changes, yeah, no intention on changing them now. (Side comment: it's always interesting to see people's different perceptions... most of your suggestions lean towards making it look more like Typescript or Go, which is probably what you're most used to! "Intuitive" is always relative to the beholder — Teal's syntax style follows on the Lua tradition, which is in itself more aligned to the Pascal/Modula family than that of curly-bracket-style languages).

@muratg
Copy link

muratg commented May 31, 2022

I agree familiarity is important in deciding what's intuitive. My main issue with colon in map and function syntax was more about : being overused. In the typing context, a colon usually comes after a variable name or a function argument. But for map it's coming after another type, for example. There's a similar issue in Typescript too. But in Lua/Teal syntax it's a slightly bigger issue, because : is also used to denote "methods", both in definition and use site.

But I can totally see your point of view too. It makes sense to target existing Lua users for Teal instead of other languages.

Looking forward to seeing the intersection types! I think it should have less limitations compared to union types since it's an extension process and not a contraction. If you already thought of its syntax, please share!

@hishamhm
Copy link
Member

hishamhm commented Jan 8, 2024

I'm closing this PR since we'll be getting functionality to address similar use-cases via the interface type as already implemented in the next branch! Thank you all for the brainstorming and feedback!

Interfaces still need some work (generics, ensuring the subtyping relations all work as intended (tests!!!!)) but I'm pretty happy with the implementation to the point that I've already ported the compiler's own type objects to use them :)

https://github.com/teal-language/tl/tree/next

@pigpigyyy
Copy link
Contributor Author

Just started testing the Next Teal! Everything is supposed to be there except the "readonly field" feature, I will try making a new PR with the newest version. Thanks for your effort!

@pigpigyyy pigpigyyy closed this Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants